Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw an error when using a Union or Interface as an argument type #1222

Merged
merged 3 commits into from
Sep 10, 2021

Conversation

MAM-SYS
Copy link
Member

@MAM-SYS MAM-SYS commented Sep 7, 2021

Hello dear friends @jkimbo @patrick91 @BryceBeagle
This patch is about raising an exception when we use invalid arguments such as Union and Interface. issue #1065
Hope it can help
If anything needs to be changed or fixed, just let me know
With Respect

Description

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

#1065

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@botberry
Copy link
Member

botberry commented Sep 7, 2021

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This release adds a new exception called InvalidFieldArgument which is raised when a Union or Interface is used as an argument type.
For example this will raise an exception:

import strawberry

@strawberry.type
class Noun:
    text: str

@strawberry.type
class Verb:
    text: str

Word = strawberry.union("Word", types=(Noun, Verb))

@strawberry.field
def add_word(word: Word) -> bool:
	...

Here's the preview release card for twitter:

Here's the tweet text:

🆕 Release (next) is out! Thanks to Mohammad Hossein Yazdani for the PR 👏

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

Copy link
Member

@jkimbo jkimbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great @MAM-SYS , thank you! I've added some suggested changes.

strawberry/exceptions.py Outdated Show resolved Hide resolved
strawberry/exceptions.py Outdated Show resolved Hide resolved
strawberry/field.py Show resolved Hide resolved
strawberry/field.py Outdated Show resolved Hide resolved
strawberry/field.py Outdated Show resolved Hide resolved
RELEASE.md Outdated Show resolved Hide resolved
RELEASE.md Outdated Show resolved Hide resolved
@MAM-SYS
Copy link
Member Author

MAM-SYS commented Sep 8, 2021

@jkimbo i appreciate you
It seems my English language is really bad. I need to study more :)
But Thanks for your help. It is very valuable to me
I fixed everything as you suggested, but if anything needs to be fixed again, just tell me
With Respect

Copy link
Member

@jkimbo jkimbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @MAM-SYS ! And your English is very good 😊

I'm going to leave this open for @patrick91 or @BryceBeagle to have a look as well before merging.

RELEASE.md Outdated
@@ -0,0 +1,21 @@
Release type: patch

This release adds a new exception called `InvalidFieldArgument` which is raised when if a Union or Interface is used as an argument type.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This release adds a new exception called `InvalidFieldArgument` which is raised when if a Union or Interface is used as an argument type.
This release adds a new exception called `InvalidFieldArgument` which is raised when a Union or Interface is used as an argument type.

Comment on lines 425 to 452
def test_union_as_an_argument_type():
with pytest.raises(InvalidFieldArgument):

@strawberry.type
class Noun:
text: str

@strawberry.type
class Verb:
text: str

Word = strawberry.union("Word", types=(Noun, Verb))

@strawberry.field
def add_word(word: Word) -> bool:
return True


def test_interface_as_an_argument_type():
with pytest.raises(InvalidFieldArgument):

@strawberry.interface
class Adjective:
text: str

@strawberry.field
def add_word(adjective: Adjective) -> bool:
return True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a test that checks it raises in this case too?

@strawberry.interface
class Adjective:
    text: str

def add_word(adjective: Adjective) -> bool:
    return True

@strawberry.type
class Mutation:
    add_word: bool = strawberry.field(resolver=add_word)

Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, just added a couple of minor comments :)

@codecov
Copy link

codecov bot commented Sep 10, 2021

Codecov Report

Merging #1222 (8023772) into main (e724177) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1222      +/-   ##
==========================================
+ Coverage   97.56%   97.58%   +0.02%     
==========================================
  Files          88       88              
  Lines        3321     3358      +37     
  Branches      477      488      +11     
==========================================
+ Hits         3240     3277      +37     
  Misses         45       45              
  Partials       36       36              

@MAM-SYS
Copy link
Member Author

MAM-SYS commented Sep 10, 2021

Thanks for your help and suggestions again @patrick91
Of course, i'll send the changes right now.

Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for the PR and for pushing the requested changes :D

@patrick91 patrick91 merged commit 0ec09a6 into strawberry-graphql:main Sep 10, 2021
@botberry
Copy link
Member

Thanks for contributing to Strawberry! 🎉 You've been invited to join
the Strawberry GraphQL organisation 😊

You can also request a free sticker by filling this form: https://forms.gle/dmnfQUPoY5gZbVT67

And don't forget to join our discord server: https://strawberry.rocks/discord 🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants